[TrimmableTypeMap] Fix UCO constructor activations#11094
Conversation
Refactor UCO constructor wrappers to match legacy ManagedPeer.Construct activation behavior: - Add WithinNewObjectScope guard: skip activation when the object is being created from managed code (JNIEnv.StartCreateInstance/NewObject), preventing double peer creation and GC crashes. - For non-leaf types (activation ctor on a base type), call ActivatePeerFromJavaConstructor at runtime instead of inline IL. This finds the matching managed ctor for the JNI signature (e.g., parameterless for "()V") and invokes it via ActivatePeer, preserving user constructor side effects like field initialization. - For leaf types, inline the activation ctor call directly in the UCO with the WithinNewObjectScope guard. - Emit no-op UCO for open generic type definitions (type params unknown). - Add ControlFlowBuilder support to PEAssemblyBuilder.EmitBody for branch instructions (useBranches parameter). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
trimmable-type-map
copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- UCO constructors directly call activation ctor (no ActivateInstance indirection) - WithinNewObjectScope guard prevents double peer creation - No-op UCO for open generic type definitions - ControlFlowBuilder support in PEAssemblyBuilder - Remove TrimmableNativeRegistration wrapper and ActivateInstance Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the trimmable typemap generator/runtime path to match legacy UCO constructor activation behavior by guarding against double peer creation and inlining the activation IL in generated nctor_*_uco wrappers (removing the previous runtime helper entrypoint).
Changes:
- Inline managed peer activation directly in generated UCO constructor wrappers, including a
JniEnvironment.WithinNewObjectScopeguard. - Remove
TrimmableTypeMap.ActivateInstance()from the runtime path. - Extend
PEAssemblyBuilder.EmitBody(...)to optionally enable branch/label emission (viaControlFlowBuilder) and add a generator test for the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Adds a regression test asserting UCO constructors reference WithinNewObjectScope + inlined activation APIs and do not reference removed helpers. |
| src/Mono.Android/Microsoft.Android.Runtime/TrimmableTypeMap.cs | Removes the now-unused ActivateInstance runtime helper. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Switches UCO constructor wrapper emission to guarded, inlined activation and adjusts supporting references. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs | Adds a useBranches option to allow emitting branching IL via ControlFlowBuilder; adjusts UTF-8 helper nested type visibility. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/Model/TypeMapAssemblyData.cs | Updates model documentation to reflect the new non-helper activation behavior. |
| encoder.OpCode (ILOpCode.Ret); | ||
| }); | ||
| metadata.AddCustomAttribute (typeDefHandle, ctorHandle, _ucoAttrBlobHandle); |
There was a problem hiding this comment.
This adds a custom attribute to the proxy type using ctorHandle (the proxy’s own .ctor MethodDef) as the attribute constructor. That produces invalid metadata (the ctor handle must point to the attribute’s ctor, e.g. _ucoAttrCtorRef), and UnmanagedCallersOnlyAttribute shouldn’t be applied to the proxy type/instance ctor anyway. Remove this AddCustomAttribute call (UCO should only be applied via AddUnmanagedCallersOnlyAttribute() on the generated wrapper methods).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
left a comment
simonrozsival
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict:
Found 1 follow-up item:
- 💡 Testing: the new UTF-8 helper pre-materialization is a subtle metadata-ordering workaround and would benefit from more targeted regression coverage (
src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs:348).
The main UCO activation change looks consistent with the intended non-reflective design. Public CI is green, but the internal Xamarin.Android-PR pipeline is still queued, so this is not ready for LGTM yet.
Review generated by android-reviewer from review guidelines.
| void EmitProxyType (JavaPeerProxyData proxy, Dictionary<string, MethodDefinitionHandle> wrapperHandles) | ||
| { | ||
| if (proxy.IsAcw) { | ||
| // RegisterNatives uses RVA-backed UTF-8 fields under <PrivateImplementationDetails>. |
There was a problem hiding this comment.
🤖 💡 Testing — this eager GetOrAddUtf8Field() loop is fixing a very subtle metadata-ordering problem in the generated assembly. Could we strengthen Generate_AcwProxy_HasRegisterNativesAndUcoMethods() to assert that RegisterNatives is owned by the proxy type, not just present somewhere in MethodDefinitions? That would lock in the exact behavior this workaround depends on and make future cleanup safer.
{Rule: Missing regression tests for subtle generator fixes}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
This PR fixes the UCO constructor activation parity regression for trimmable typemaps in one review.
What it does
JniEnvironment.WithinNewObjectScopeguard in generatednctor_*_ucowrappers to avoid double peer creation from managed construction pathsActivatePeerFromJavaConstructor()path entirelyTrimmableTypeMap.ActivateInstance()indirectionPEAssemblyBuilder.EmitBody(..., useBranches: true)) and theRegisterNativesUTF-8 helper materializationNon-goals
Notes